Skip to content

✨ fix(reviewer-lane): true creation-time ordering + structured re-escalation hand-off (#5617 items 3–4) - #5682

Merged
kubestellar-prow[bot] merged 3 commits into
hivecommons:v5from
Danathar:feat/5617-reviewer-lane-followups
Sep 2, 2026
Merged

✨ fix(reviewer-lane): true creation-time ordering + structured re-escalation hand-off (#5617 items 3–4)#5682
kubestellar-prow[bot] merged 3 commits into
hivecommons:v5from
Danathar:feat/5617-reviewer-lane-followups

Conversation

@Danathar

@Danathar Danathar commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Two of the four follow-ups deferred from the #5480 reviewer-lane delivery. Both are small, independent, and self-contained; neither changes who is allowed to do what.

  • The reviewer was working the wrong PRs first. Its work list says "oldest first" and is capped at three PRs per kick, but the rows had no age information at all — they were sorted by repo name, then PR number. On a hive with more than one repo that means an old escalated PR in a late-alphabet repo can sit behind newer ones forever. The PR's real creation time was already being fetched; it just was not being written into the file the list is built from. Now it is.
  • A human inheriting a twice-failed PR was told nothing about the first attempt. The reviewer lane gets exactly one pass per PR: once it has repaired one, that PR is excluded from its work list permanently, so when the PR goes red again the escalation comment is the last thing the machinery will ever say about it. That comment was the same generic body a first escalation gets — the only thing marking it as a hand-off was the label set. It now says a reviewer already had its pass, which commit it left behind, and that nothing automated is coming next.

Item 1 (G2 pending-wipe) shipped in #5634. Item 2 (the reviewer-lane.md kick template) is not claimed here — see below.

Fix

Item 4 — true creation-time ordering

formatReviewerWorkList ordered on (repo, number) because ci-failing.json carried no creation time. PR numbers are monotonic only within a repo, so that key sorted by repo name first. Combined with the reviewerMaxPRsPerKick cap this is a starvation bug, not a cosmetic one: a month-old escalated PR in zeta/service sits behind three newer ones in alpha/console on every kick, forever, and the reviewer never reaches it.

github.PullRequest.CreatedAt was already populated at enumeration and simply never threaded through. writeMergeEligible now records it; the work list orders on it and renders it as an opened: line, so the reviewer can verify the ordering itself — the kick's INVARIANTS forbid gh pr list.

Rows with no creation time (a ci-failing.json written by an older hub, or a forge that omitted the field) sort last and keep the old proxy among themselves. An unproven age must not jump ahead of a measured one, and this makes the change a no-op on a stale artifact rather than a reshuffle.

Item 3 — structured hand-off note

The hub never observes a reviewer verdict directly: it is a label edit, usually a direct gh pr edit. The single moment it can see one is Sweep's reviewer-verdict reconciliation (#5511, gap G1) — so the record is taken there.

  • Entry gains ReviewerPassedSHA / ReviewerPassedAt, stamped by that reconciliation and deliberately surviving the ledger reset the same reconciliation performs. Keyed on the SHA, so a repeat reconciliation cannot walk the timestamp forward and misdate the hand-off. Cleared only with the entry itself — a PR that goes green has converged and starts a fresh story.
  • Store.ReviewerPass exposes it; runEscalationSweep uses it to pick HandoffCommentBody over CommentBody.

The note gives the head commit the reviewer left, when the PR returned to the automated lane, that the attempt count is measured from that verdict (the ledger restarts there), and that no further automated pass is coming. It points at the reviewer's own audited record — the Reviewer adjudication: relay comment attributed agent_pr_reviewed, plus the advisory bead — rather than restating it: the hub never saw the reviewer's reasoning, so summarising it would be invention. The ntfy page is retitled on this path only.

First escalations are byte-for-byte unchanged, and a partial record (SHA or timestamp missing) omits the field rather than rendering an empty backtick pair or 0001-01-01.

Cluster claimed (files/functions)

  • src/pkg/scheduler/reviewer_lane.goformatReviewerWorkList ordering key + opened: row.
  • src/cmd/hive/main.gowriteMergeEligible's failingPR.CreatedAt; runEscalationSweep's comment/notify selection.
  • src/pkg/escalation/escalation.goEntry reviewer-pass fields, Sweep stamping, Store.ReviewerPass, ReviewerHandoff + HandoffCommentBody (CommentBody keeps its signature and behavior).
  • src/docs/adr/0010-escalation-circuit-breaker.md, CHANGELOG.md.
  • Tests: 2 ledger/body tests (pkg/escalation), 2 ordering tests (pkg/scheduler), 1 end-to-end sweep test (cmd/hive).

Disjoint from the open hold-gated PRs. The nearest neighbour is #5670, which also edits runEscalationSweep — it adds a lifecycleRecorder parameter and a recordBlocked call between MarkEscalated and logger.Info. This PR touches the comment construction above that and the notifier block below it, and leaves logger.Info alone specifically so the two do not collide. Whichever lands second should merge cleanly; if not, the resolution is to keep both.

Validation

  • go build ./..., go vet ./cmd/hive/ ./pkg/escalation/ ./pkg/scheduler/, and gofmt -l clean on every touched file.
  • go test ./cmd/hive/ ./pkg/escalation/ ./pkg/scheduler/ passes.
  • Both new behaviors were mutation-checked: reverting the ordering key and short-circuiting the hand-off selection each make the corresponding new test fail, so neither passes vacuously.
  • The first commit was checked out and built/tested on its own, so the series is bisectable.
  • No formal-model change: the ledger records a new field but Sweep's state machine and transitions are untouched, so src/formal/escalation is unaffected.

Not claimed

Item 2 (embedded reviewer-lane.md kick template) is deliberately left for a separate PR. It is a change to the kick resolution chain rather than to the lane's data, and the rendered template has to reproduce the ~25 safety strings pinned by TestBuildReviewerMessage_ContractAtL5 — including the hard ACMM dormancy gate, which must stay in Go so an operator-editable template cannot switch the lane on below L5. That deserves its own review, not a rider on this one.

Related issues

Refs #5617 (items 3 and 4 of 4 — does not close the tracker)

Testing

  • cd src && go build ./...
  • cd src && go test ./... — ran the three affected packages (./cmd/hive/, ./pkg/escalation/, ./pkg/scheduler/); all pass.

— hive: backend=claude model=claude-opus-5

…PR number

Deferred item 4 of hivecommons#5617. The reviewer lane's work list is capped at
reviewerMaxPRsPerKick rows and documented "oldest first", but the rows carried
no age signal at all: ci-failing.json recorded no creation time, so
formatReviewerWorkList ordered by (repo name, PR number).

PR numbers are monotonic only WITHIN a repo, so that proxy sorted by repo NAME
first. Against the per-kick cap that is a starvation bug, not a cosmetic one —
a month-old escalated PR in "zeta/service" sits behind three newer ones in
"alpha/console" on every kick, forever, and the reviewer never reaches it.

github.PullRequest already carried CreatedAt from enumeration; it was simply
never threaded into the ci-failing rows. writeMergeEligible now records it and
formatReviewerWorkList orders on it, rendering the key as an `opened:` line so
the reviewer can verify the ordering without `gh pr list` (which the kick's
INVARIANTS forbid).

Rows with no creation time — a ci-failing.json written by an older hub, or a
forge that omitted the field — sort LAST and keep the old (repo, number) proxy
among themselves: an unproven age must not jump ahead of a measured one. That
also makes the change a no-op on a stale artifact rather than a reshuffle.

Refs hivecommons#5617 (item 4 of 4 — does not close the tracker)

Signed-off-by: Doug Baggett <doug.baggett@gmail.com>
…fter a reviewer pass

Deferred item 3 of hivecommons#5617. The reviewer lane is a one-pass ladder: a PR carrying
`reviewer-passed` is excluded from the reviewer work list permanently, so when
it goes red again the escalation comment is the LAST thing the machinery will
ever say about it. That comment was the generic first-escalation body — nothing
but the label set distinguished a terminal hand-off from a first escalation,
and nothing told the human that a mechanical repair had already been tried and
had not held.

The hub never observes a reviewer verdict directly (it is a label edit, usually
a direct `gh pr edit`); the only moment it can see one is Sweep's
reviewer-verdict reconciliation (hivecommons#5511, gap G1). So record it there:

- Entry gains ReviewerPassedSHA/ReviewerPassedAt, stamped by the reconciliation
  and deliberately SURVIVING the ledger reset that same reconciliation
  performs. Keyed on the SHA so a repeat reconciliation cannot walk the
  timestamp forward and misdate the hand-off. Cleared only with the entry
  itself — a PR that goes green has converged and starts a fresh story.
- Store.ReviewerPass exposes it; runEscalationSweep uses it to choose between
  CommentBody and the new HandoffCommentBody.

The note states the head commit the reviewer left on the branch, when the PR
was returned to the automated lane, that the attempt count is measured FROM
that verdict (the ledger restarts there), and that no further automated pass is
coming. It points at the reviewer's own audited record — the
`Reviewer adjudication:` relay comment attributed as `agent_pr_reviewed`, plus
the advisory bead — rather than restating it: the hub never saw the reviewer's
reasoning, so summarising it would be invention.

The ntfy page is retitled on this path too; a hand-off with nothing automated
behind it is materially more urgent than a first escalation.

Refs hivecommons#5617 (item 3 of 4 — does not close the tracker)

Signed-off-by: Doug Baggett <doug.baggett@gmail.com>
…ow-ups

Both changes are user-visible: the escalated-PR work list changes order, and
the second-escalation PR comment a human reads changes content.

Refs hivecommons#5617

Signed-off-by: Doug Baggett <doug.baggett@gmail.com>
@kubestellar-prow kubestellar-prow Bot added dco-signoff: yes Indicates the PR's author has signed the DCO. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 2, 2026

@clubanderson clubanderson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — OK to merge.

Verified against the v5 base (correct branch for the lane) and the #5617 tracker:

  • Item 4 (creation-time ordering): CreatedAt is threaded end-to-end — writeMergeEligible records pr.CreatedAt into the ci-failing.json row, and formatReviewerWorkList sorts on it. The comparator is a valid strict weak order: known-age rows first by time, ties and unknown-age rows fall back to the old (repo, number) proxy, so a stale artifact is a no-op rather than a reshuffle. The rendered opened: line makes the key auditable from inside the kick. Both ordering tests encode the actual starvation scenario (late-alphabet repo, oldest PR) and fail under the old key.
  • Item 3 (hand-off note): the stamp is taken at the only observable moment (Sweep's #5511 G1 reviewer-verdict reconciliation), survives the ledger reset it accompanies, is SHA-keyed so a repeat reconciliation cannot walk the timestamp, and is cleared only with the entry (green = fresh story, as documented). runEscalationSweep routes to HandoffCommentBody solely on ReviewerPass ok; first escalations are byte-identical. The body's claims match the verified ledger semantics: attempts measured from the verdict (test pins Attempts == 1 after reconciliation) and permanent reviewer-passed exclusion from the work list (one-pass invariant).
  • Formal model: formal-verify (escalation) ran and passed on the head SHA (it's beyond the first check-runs page — confirmed via the paginated API). Sweep's transitions are untouched; only fields were added, so no model drift.
  • No double-audit with #5588: the diff emits no new audit events — the hand-off note points at the existing Reviewer adjudication: relay comment / advisory bead rather than re-recording them. Only the ntfy title changes, and only on the hand-off path.
  • Partial-record rendering (no empty backticks, no 0001-01-01) is tested; the #5670 overlap in runEscalationSweep is disjoint as described.

Minor, non-blocking: CreatedAt has no omitempty, so a forge-omitted time serializes as the zero time — harmless, since the reader treats zero as unknown and sorts it last.

@clubanderson clubanderson added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 2, 2026
@kubestellar-prow

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has signed the DCO. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants